feat(mcp): preserve CallToolResult.isError flag in MCPToolResult#2118
feat(mcp): preserve CallToolResult.isError flag in MCPToolResult#2118Zelys-DFKH wants to merge 2 commits intostrands-agents:mainfrom
Conversation
Adds an optional isError field to MCPToolResult, set to True only when the underlying MCP tool returned CallToolResult.isError=True. This lets callers tell apart application-level errors from protocol/transport errors, which currently both surface as status='error'. Closes strands-agents#1670
Zelys-DFKH
requested a deployment
to
manual-approval
GitHub Actions
Waiting
Zelys-DFKH
requested a deployment
to
manual-approval
GitHub Actions
Waiting
|
/strands review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| structuredContent: NotRequired[dict[str, Any]] | ||
| metadata: NotRequired[dict[str, Any]] | ||
| isError: NotRequired[bool] |
There was a problem hiding this comment.
Issue: The type NotRequired[bool] allows both True and False as values, but per the implementation and docstring contract, only True is ever set (and the key is absent otherwise). Using NotRequired[Literal[True]] would encode the actual contract in the type system — the field is either absent or True, never False.
Suggestion: Consider narrowing the type to NotRequired[Literal[True]] to make the semantics explicit:
from typing import Literal
isError: NotRequired[Literal[True]]This helps callers understand they can simply check if result.get("isError") without worrying about an explicit False value.
|
Assessment: Comment (leaning Approve) Clean, well-scoped change that correctly addresses #1670 by preserving the MCP Review Details
The docstring clearly explains the three-way semantics (tool error vs. protocol error vs. success), and the change is minimal and well-tested. Nice contribution! 👍 |
| # No structured content should be present when not provided by MCP | ||
| assert result.get("structuredContent") is None | ||
| # isError flag should be True only when the tool reported an application error | ||
| assert result.get("isError") == (True if is_error else None) |
There was a problem hiding this comment.
Issue: The assertion result.get("isError") == (True if is_error else None) works but is slightly unclear — when is_error=False, it's asserting None == None, which doesn't clearly communicate that the key should be absent.
Suggestion: For the is_error=False case, explicitly assert key absence to match the documented contract:
if is_error:
assert result.get("isError") is True
else:
assert "isError" not in resultThis more directly tests the documented behavior (key absent on success vs. True on error).
| result["structuredContent"] = call_tool_result.structuredContent | ||
| if call_tool_result.meta: | ||
| result["metadata"] = call_tool_result.meta | ||
| if call_tool_result.isError: |
There was a problem hiding this comment.
we should check isError != None so we pass through false values too
There was a problem hiding this comment.
Good catch. Updated to if call_tool_result.isError is not None: result["isError"] = call_tool_result.isError so both True and False come through. The key is only absent when a protocol/client exception bypasses _handle_tool_result entirely. Tests updated to match.
Per maintainer feedback: use 'if call_tool_result.isError is not None' so both True and False are preserved in the result. The field is absent only when a protocol/client exception bypasses _handle_tool_result. Updated test assertions to match.
Zelys-DFKH
requested a deployment
to
manual-approval
GitHub Actions
Waiting
Zelys-DFKH
requested a deployment
to
manual-approval
GitHub Actions
Waiting
2 participants
Description
Adds an optional
isErrorfield toMCPToolResult, set toTrueonly when the underlying MCP tool returnedCallToolResult.isError=True. This lets callers tell apart application-level errors (the tool ran but reported a failure) from protocol/transport errors (the tool never ran because of a client exception, timeout, or session issue). Right now both surface identically asstatus="error", which is what #1670 is asking to fix.Behavior:
CallToolResult.isError=True): result hasstatus="error"andisError=Truestatus="error"and noisErrorkey. This path goes through_handle_tool_execution_errorand is unchanged.status="success", noisErrorkeyThe field is
NotRequired, so existing code readingMCPToolResultkeeps working without changes.Related Issues
Closes #1670
Documentation PR
N/A
Type of Change
New feature
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli
test_call_tool_sync_statusandtest_call_tool_async_statusparametrized tests to assertresult.get("isError") == (True if is_error else None)test_mcp_tool_result_typeto confirm the field is absent by default and settable when providedhatch run prepare(ruff + mypy + full test suite): 2524 passed, 4 skipped, no new warningsNo new docs needed — this is a small additive field on an existing TypedDict, already documented in the docstring on
MCPToolResult.hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.